Seperate adding tasks to cgroups and applying resource restrictions #111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
utam0k
reviewed
Jun 26, 2021
| @@ -19,7 +19,8 @@ pub const CGROUP_PROCS: &str = "cgroup.procs"; | |||
| pub const DEFAULT_CGROUP_ROOT: &str = "/sys/fs/cgroup"; | |||
|
|
|||
| pub trait CgroupManager { | |||
| fn apply(&self, linux_resources: &LinuxResources, pid: Pid) -> Result<()>; | |||
| fn add_task(&self, pid: Pid) -> Result<()>; | |||
There was a problem hiding this comment.
This is slightly out of the scope of this PR, However Could you provide comments on each function?
utam0k
reviewed
Jun 26, 2021
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| impl Blkio { | ||
| fn apply(root_path: &Path, blkio: &LinuxBlockIo) -> anyhow::Result<()> { | ||
| fn apply(root_path: &Path, blkio: &LinuxBlockIo) -> Result<()> { |
nimrodshn
reviewed
Jun 26, 2021
src/cgroups/v2/systemd_manager.rs
Outdated
| @@ -45,6 +46,19 @@ struct CgroupsPath { | |||
|
|
|||
| impl SystemDCGroupManager { | |||
| pub fn new(root_path: PathBuf, cgroups_path: PathBuf) -> Result<Self> { | |||
| // TODO: create the systemd unit using a dbus client. | |||
| let cgroups_path = Self::new_cgroups_path(cgroups_path)?; | |||
| let cgroups_path = Self::get_cgroups_path(cgroups_path)?; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Was not super happy with the naming either. Reworked it. Let me know what you think.
nimrodshn
reviewed
Jun 26, 2021
src/cgroups/v2/systemd_manager.rs
Outdated
| common::write_cgroup_file(full_path.join(CGROUP_PROCS), &pid)?; | ||
| Ok(full_path) | ||
| common::write_cgroup_file(self.full_path.join(CGROUP_PROCS), &pid)?; | ||
| Ok(()) |
There was a problem hiding this comment.
I wonder if this method can just end with common::write_cgroup_file(self.full_path.join(CGROUP_PROCS), &pid)
|
@Furisto Just to make sure I follow: |
|
BTW do you mind squashing commits? |
- Add comments for functions - Use better naming in systemd cgroup manager
|
@nimrodshn Yes, correct. Add_task will move a process into the cgroup and construct the path if it doesn't exist. Apply will apply the resource restrictions. The create command would use add_task and apply, while exec would only use add_task because here we are moving in a cgroup that has already been set up. I folded the last commit, which was indeed a bit trivial into my new commit. I would prefer to keep the other ones though because they deal with separate things. |
@Furisto Just out of curiosity how does the |
ferrell-code
added a commit
to ferrell-code/youki
that referenced
this pull request
Jul 18, 2021
add default handling when there isn't cgroup path in config.json. organize the logger. Merge pull request youki-dev#47 from utam0k/organize-log organize the logger. Merge pull request youki-dev#45 from utam0k/no-cgroup-path add default handling when there isn't cgroup path in config.json. ignore submodule artifacts. add a comment in oci_spec. make sure that config.json can be read regardless of where it is executed. add the tutorial on using youki. Merge pull request youki-dev#49 from utam0k/tutorial add the tutorial on using youki. update README. Merge pull request youki-dev#50 from utam0k/edit-readme update README. Reorganize cgroup modules cpu and mems is optional according to spec Spike cgroupv2 implementation Move CgroupManager to common module Implement cgroup manager trait Detect group version and create manager Improve file write options Implement string trait for controller types style Support cpu controller Add test module Style Enable controllers along the hierarchy Add tests for cpu controller Add modules for controllers Add tests for cpuset controller Use Result consistently Rework cgroup fs detection Support removal of cgroup Remove types module Formatting only Prefer v1 over v2 Fix integration test failure Control selection of cgroup fs with env variable Get rid of warnings Merge pull request youki-dev#48 from Furisto/cgroupv2 Initial support for cgroups v2 cargo clippy. make log level debug to get more information when ci failed. Merge pull request youki-dev#53 from utam0k/fix-unstable-ci make log level debug to get more information when ci failed. 4b260b0 Merge branch 'main' of github.com:utam0k/youki into HEAD Merge pull request youki-dev#52 from utam0k/cargo-clippy cargo clippy. Align cgroup controller implementations - Use common method to write to cgroup file - Ensure that pid is always written to the cgroup Merge pull request youki-dev#54 from Furisto/cg-common Align cgroup controller implementations add spec cli command resolve conflicts impl defaults for lots of structs Consolidate cgroup test methods Merge pull request youki-dev#57 from Furisto/cg-tests Consolidate cgroup test methods Update README.md - Change comment out sign `//` to `#` in shell script - Add new line at end of file formatting add back default attributes Update comment Merge branch 'main' of github.com:utam0k/youki into main Update comment Merge branch 'main' of github.com:YJDoc2/youki into main Add 'Community' section to README.md Merge pull request youki-dev#59 from nimrodshn/add_community_section_to_readme Add 'Community' section to README.md Update README: Split code section in tutorial - Split code section in tutorial. Merge pull request youki-dev#58 from aoki/main Update README.md Update comment, fix cargo-clippy warning Merge pull request youki-dev#43 from YJDoc2/main Add comments to create.rs utam0k -> containers Signed-off-by: Sora Morimoto <sora@morimoto.io> Merge pull request youki-dev#61 from smorimoto/utam0k-containers utam0k -> containers Add cgroup v1 cpu controller Add cgroup v1 cpuset controller Unit tests for cgroup v1 cpu controller Unit tests for cgroup v1 cpuset controller Cut down on boilerplate Activate cpu integration test Fix error while moving task into cgroup If a task is moved into the cgroup and a value has not been set for cpus and mems Errno 28 (no space left on device) will be returned. Therefore we set the value from the parent if required. Improve reliability of cgroup removal Merge pull request youki-dev#63 from Furisto/cg-cpu-v1 Support for cgroup v1 cpu and cpuset subsystem Add comments to process module and minor refactoring Changed hard coded mentions of poll time and event numbers to references of constants Merge pull request youki-dev#64 from YJDoc2/main Add comments to process module and minor refactoring Added install command for prerequisite in README Merge pull request youki-dev#66 from PeterYordanov/add-install-prerequisite Added install command for prerequisite in README Fixed spelling mistake in src/rootfs.rs Merge pull request youki-dev#67 from PeterYordanov/spelling-mistake-rootfs Fixed spelling mistake in src/rootfs.rs add handling of WouldBlock error. Use init pid instead of child pid Ensure controllers are enabled at the root Logging Added folder structure section in README Added folder structure section in README Added resources to folder structure section Fixed spelling mistakes Added documentation comment for cgroup module Added doc comments for modules Removed folder structure section Update doc comment Merge pull request youki-dev#68 from utam0k/handle_wouldblock add handling of WouldBlock error. Create a template for integration tests. refactore Run fmt Merge branch 'main' into add-spec-arg Added doc on how to run integration tests Update youki path Change build command Merge pull request youki-dev#73 from minakawa-daiki/fix-youki-path Change execution path and fix CI Merge branch 'main' into add-integration-template Merge pull request youki-dev#69 from Furisto/cg-escape Fix issues with cgroup v1 and v2 Merge pull request youki-dev#71 from minakawa-daiki/add-integration-template Added Integration test template basic device tests property testing on devices property testing hugetbl and memory controllers fix comount checks Updated doc comments migrate LinuxCapabilityType to enum Update caps_migrations.rs Fixed typo Unfinished sentence Merge pull request youki-dev#70 from PeterYordanov/added-doc-comments-modules Added doc comments modules Add comments to container module make LinuxCapabilityType add some widgets to README.md Merge pull request youki-dev#76 from utam0k/widget add some widgets to README.md Handle relative cgroup paths Ensure parents have value Merge pull request youki-dev#74 from Furisto/cg-relative Handle relative cgroup paths address requested changes Merge pull request youki-dev#75 from tsturzl/main Improved testing, property testing, device tests impl From for Capabilities and add tests Add comments to command module Add draft-doc for container and command module Merge branch 'main' of github.com:containers/youki into main Merge pull request youki-dev#79 from YJDoc2/main Document Container and Command modules Fix README badges fix github ci badge typo... Merge pull request youki-dev#80 from tsturzl/main Fix badges in README add create kill delete state in integration test Merge pull request youki-dev#81 from duduainankai/add-integration-test add create kill delete state in integration test Provide better error messages Merge pull request youki-dev#84 from Furisto/cg-better-errors Provide better error messages limit scope of unsafe get rid of unneeded unsafes make sure any values with `=` are not affected Merge pull request youki-dev#85 from tsturzl/main Clean up use of unsafe Add info command Merge pull request youki-dev#83 from Furisto/info-cmd Add info command Merge remote-tracking branch 'upstream/main' into add-spec-arg resolve conflict add lots of comments Add CODE-OF-CONDUCT.md and SECURITY.md Fix README link typo Merge pull request youki-dev#88 from sasurau4/fix/readme-link Fix README link typo Merge pull request youki-dev#86 from utam0k/coc-security Add CODE-OF-CONDUCT.md and SECURITY.md docs clean up around the tty. Renamed Cond to Pipe Merge pull request youki-dev#89 from utam0k/tty-refactor clean up around the tty. Merge pull request youki-dev#90 from YJDoc2/main Rename Cond to Pipe make sure to log any unimplemented controllers. Merge pull request youki-dev#91 from utam0k/unknown-controller make sure to log any unimplemented controllers. Add support for cpuacct in cgroup v1. Remove unnecessary processing. use bail! insted of anyhow Merge pull request youki-dev#92 from yjuba/v1-cpuacct [WIP] Add support for cpuacct in cgroup v1. Merge pull request youki-dev#94 from utam0k/bail use bail! insted of anyhow Add cgroup v1 freezer controller Add a test for applying CpuAcct. Merge pull request youki-dev#96 from yjuba/add-cpuacct-test Add a test for applying CpuAcct. improve build time in CI Check if rootless container is required and ensure prerequisites Ensure map binaries are available Implement protocol for identifier mapping Ensure root directory can be written by non root user Identifier mapping names were not correct Only one mapping needs to match Prevent panic when resources are not specified Fix wrong mapping Clippy and fmt Merge pull request youki-dev#93 from duduainankai/add-freezer Add cgroup v1 freezer controller remove the cargo-when dependency. Address review comments - should_use_rootless doesn't need Result type - add warning regarding current rootless limitations - make lookup_map_binary more concise Merge pull request youki-dev#98 from Furisto/rootless Experimental support for rootless containers Add unit tests for tty module Reuse tmpdir implementation from cgroup Extend info cmd with version and os Merge pull request youki-dev#102 from constreference/tty Add unit tests for tty module Merge pull request youki-dev#101 from Furisto/info-ex Extend info cmd with version and os Use `assert!` instead of `assert_eq!` when comparing a boolean. Merge pull request youki-dev#104 from utam0k/fix-clippy Use `assert!` instead of `assert_eq!` when comparing a boolean. Add support for systemd managed cgroups Merge pull request youki-dev#46 from nimrodshn/add_support_for_systemd_cgroups Add support for systemd managed cgroups update README.md Merge pull request youki-dev#105 from utam0k/update-README update README.md clean up comments Fix README.md Fedora & Centos instructions Merge pull request youki-dev#107 from nimrodshn/fix_readme Fix README.md Fedora & Centos instructions Add list command Merge pull request youki-dev#108 from Furisto/list-cmd Add list command fix conflicts. Merge pull request youki-dev#97 from utam0k/actions-cache improve build time in CI split the subcommands into their own files. Merge pull request youki-dev#110 from utam0k/refactor-cli-commands split the subcommands into their own files. Seperate adding tasks and applying resource restrictions Shorten names Update README.md Change `dnf` to `apt-get` for Debian based systems Address review comments - Add comments for functions - Use better naming in systemd cgroup manager Merge pull request youki-dev#112 from bkochendorfer/patch-1 Update README.md Merge pull request youki-dev#111 from Furisto/cg-add Seperate adding tasks to cgroups and applying resource restrictions Require only cgroups that are needed to fullfill the resource restrictions Merge pull request youki-dev#114 from Furisto/cg-only-required Require only requested cgroups to be present force delete container if it is running or created Merge pull request youki-dev#115 from TinySong/main force delete container if it is running or created add comments in intergration_test.sh about test case that runc no paas Merge pull request youki-dev#116 from TinySong/main add comments in intergration_test.sh about test case that runc no paas remove unnecessary clone() in create.rs Merge pull request youki-dev#117 from utam0k/refactor-create-remove-unnecessary-clone remove unnecessary clone() in create.rs Allow wider range of arguments for spec loading Rename command Provide context in case of errors during dir creation Ensure file info is captured Modularize create code add cgroup v2 pid controller Merge pull request youki-dev#119 from TinySong/main add cgroup v2 pids controller make String to signal conversion more user friendly by using a Trait. add tests for the signal. Split container builder into dedicated init and tenant builders The current monolithic builder provides options that should only be called during init and not when creating a tenant and vice versa. This puts the burden on the user of the builder to know which methods are safe to call. Now the ContainerBuilder can be used to specify options that are common to both scenarios and afterwards as_init/as_tenant can be called to provide scenario specific options. This also simplifies the whole "if init then else" branching logic during container build. Add documentation Remove tests Renaming Fix kill cmd test failures Execute doc tests Merge pull request youki-dev#122 from utam0k/refactor-signal make String to signal conversion more simplify by using a Trait. Review feedback and fmt Reduce binary size Merge pull request youki-dev#124 from Furisto/release Reduce size of binary Add cgroup v2 freezer controller Merge pull request youki-dev#123 from duduainankai/add-freezer-v2 Add cgroup v2 freezer controller Merge pull request youki-dev#121 from Furisto/builder Modularize container creation fix the warnings shown by cargo clippy support cgroupv2 io controller Merge pull request youki-dev#128 from TinySong/cgroupv2-io-controller Cgroupv2 io controller Merge pull request youki-dev#127 from utam0k/cargo-clippy-2 fix the warnings shown by cargo clippy Add code format check in CI format code to pass CI check Merge pull request youki-dev#129 from duduainankai/add-format-check-ci Add format check ci Fix spec path in delete Merge pull request youki-dev#130 from duduainankai/fix-path-in-delete Fix spec path in delete Document Capabilities and refactor its drop_privileges function Merge branch 'main' of github.com:containers/youki into comment-capabilities Fix same tmp dir in freezer v2 tests Merge pull request youki-dev#133 from duduainankai/fix-tmp-dir-in-test Fix same tmp dir in freezer v2 tests Merge pull request youki-dev#131 from YJDoc2/comment-capabilities Document capabilities rs and refactor its drop_privileges function cgroupsv2 hugetlb remove regex usage from hugetlb v2 use different temp dir for hugetlbv2 tests Document Info module Update doc-draft.md Merge pull request youki-dev#136 from YJDoc2/main Document Info module Document list module Merge pull request youki-dev#135 from 0xdco/cgroups-v2-hugetlb cgroupsv2 hugetlb Comment and modify Log module: Now the default log level is set according to type of compilation debug/production Merge branch 'main' of github.com:containers/youki into main Merge pull request youki-dev#137 from YJDoc2/main Document list and logger modules Implement exec command Support calling exec multiple times Fix test failure Remove todo Add comments Merge pull request youki-dev#138 from Furisto/exec Implement exec command Add pause and resume command conflicts Merge branch 'main' into add-spec-arg final issue split into multiple files add serde defaults to fields Merge pull request youki-dev#139 from duduainankai/support-pause-resume Add pause and resume command Merge branch 'main' into add-spec-arg
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is part of #20. We need to be able to move a task into a cgroup without having to apply resource restrictions again.